Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

move setImmediate from timer to uv_check #3872

Closed
wants to merge 2 commits into from

Conversation

shigeki
Copy link

@shigeki shigeki commented Aug 16, 2012

uv_check is the robust place to invoke setImmediate callbacks after process.nextTick and before timers(setTimeout/setInterval) for the following two reason.

1. immediateTimer.start(0, 1) does not keep zero timeout value

The following test of setImmediate recursion shows its callbacks need time more than 1msec at every time. This is because of uv_timer_again() as commented in joyent/libuv@90a75b0#include-uv-h-P19 This would cause missing invocation of callbacks immediately after uv_poll().

function recur(n) {
  var i = 0, max = n * 100, start = Date.now();
  (function f() {
    if ( i++ < max) {
      setImmediate(f);
    } else {
      var etime = Date.now() - start;
      console.log('iter=', max,', elapsed time=', etime , ', ave=', etime/max);
      if (n < 10) recur(++n);
      return;
    }
  })();
}
recur(1);

The resut is

> ./node ~/setImmedaite_recur_test.js
iter= 100 , elapsed time= 130 , ave= 1.3
iter= 200 , elapsed time= 263 , ave= 1.315
iter= 300 , elapsed time= 397 , ave= 1.3233333333333333
iter= 400 , elapsed time= 528 , ave= 1.32
iter= 500 , elapsed time= 650 , ave= 1.3
iter= 600 , elapsed time= 780 , ave= 1.3
iter= 700 , elapsed time= 900 , ave= 1.2857142857142858
iter= 800 , elapsed time= 1039 , ave= 1.29875
iter= 900 , elapsed time= 1179 , ave= 1.31
iter= 1000 , elapsed time= 1308 , ave= 1.308

2. callback order changes when timeout value collides with another timers

It has a possibility for setImmediate to have the same timeout value as that of another timers. If this happens, the callback order is defined by the value of pointer of handles as
https://github.com/joyent/libuv/blob/master/src/unix/timer.c#L30-33
I think that setImmediate callback should be invoked after process.nextTick and before another timers. If not, the order has to be absolutely defined.

In both cases above, we can resolve them by fixing libuv, but I think that it is best to move the place of setImmediate to uv_check where old process.nextTick was invoked.


TryCatch try_catch;

cb->Call(process, 0, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use MakeCallback() here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.I fixed it and force updated the commit. Please review it again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis I found that MakeCallback() invokes process.nextTick() just after its execution while callbacks of setTimeout/setInterval does not. This might not cause any problems but there exists a different behavior between setImmediate and timers.

@bnoordhuis
Copy link
Member

@tjfontaine You may want to chime in here.

@tjfontaine
Copy link

I guess the question is if IO should try and be serviced before the first queued immediate, if there are multiple queued immediates I don't see the placement explicitly after nextTick providing much besides saving a global timer allocation.

As far as the 1ms displacement, we could requeue the timer instead of relying on libuv to do it.

diff --git a/lib/timers.js b/lib/timers.js
index 897b64f..1009776 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -281,7 +281,7 @@ Timeout.prototype.close = function() {


 var immediateTimer = null;
-var immediateQueue = { started: false };
+var immediateQueue = {};
 L.init(immediateQueue);


@@ -294,10 +294,7 @@ function lazyImmediateInit() { // what's in a name?

 function processImmediate() {
   var immediate;
-  if (L.isEmpty(immediateQueue)) {
-    immediateTimer.stop();
-    immediateQueue.started = false;
-  } else {
+  if (!L.isEmpty(immediateQueue)) {
     immediate = L.shift(immediateQueue);

     if (immediate.domain) immediate.domain.enter();
@@ -305,6 +302,8 @@ function processImmediate() {
     immediate._onTimeout();

     if (immediate.domain) immediate.domain.exit();
+
+    immediateTimer.start(0, 0);
   }
 };

@@ -325,8 +324,7 @@ exports.setImmediate = function(callback) {

   if (!immediateQueue.started) {
     lazyImmediateInit();
-    immediateTimer.start(0, 1);
-    immediateQueue.started = true;
+    immediateTimer.start(0, 0);
   }

   if (process.domain) immediate.domain = process.domain;
@@ -343,9 +341,4 @@ exports.clearImmediate = function(immediate) {
   immediate._onTimeout = undefined;

   L.remove(immediate);
-
-  if (L.isEmpty(immediateQueue)) {
-    immediateTimer.stop();
-    immediateQueue.started = false;
-  }
 };

The results from the test script then are

iter= 100 , elapsed time= 3 , ave= 0.03
iter= 200 , elapsed time= 4 , ave= 0.02
iter= 300 , elapsed time= 1 , ave= 0.0033333333333333335
iter= 400 , elapsed time= 1 , ave= 0.0025
iter= 500 , elapsed time= 2 , ave= 0.004
iter= 600 , elapsed time= 2 , ave= 0.0033333333333333335
iter= 700 , elapsed time= 2 , ave= 0.002857142857142857
iter= 800 , elapsed time= 2 , ave= 0.0025
iter= 900 , elapsed time= 3 , ave= 0.0033333333333333335
iter= 1000 , elapsed time= 2 , ave= 0.002

@shigeki
Copy link
Author

shigeki commented Aug 17, 2012

@tjfontaine I drew a figure below to show the semantics of setImmediate in this PR. I think it is clearly understandable for the order of execution.

setImmediate semantics

@tjfontaine
Copy link

I understand the flow, I just don't necessarily see how this influences much. Unless--as I said before--it's important that we service IO before the execution of the first queued immediate

@shigeki
Copy link
Author

shigeki commented Aug 17, 2012

@tjfontaine I think the problems would occur between setImmediate and other timers(setTimeout/setInterval). In the current setImmediate, before fix 1msec delay, the following code shows an odd result.

var i = 0, j = 0, max = 5;
var tmout = setInterval(function() {
  console.log('setInterval :', i++);
  if (i > max) clearTimeout(tmout);
}, 0);
(function f() {
  setImmediate(function () {
    console.log('setImmediate :', j++);
    if (j > max) return;
    f();
  });
})();
> ./node ~/test.js
setImmediate : 0
setInterval : 0
setInterval : 1
setImmediate : 1
setInterval : 2
setImmediate : 2
setInterval : 3
setImmediate : 3
setInterval : 4
setImmediate : 4
setInterval : 5
setImmediate : 5

The order of execution is changed in Linux. On Windows it varies time by time. This is because setImmediate and setInterval have the same timeout value and depends its order of execution on their pointer value.

I tried to put start_id on timer handles at every uv_timer_start for the second index to compare each elements in RBTree of timers but I found that moving setImmediate from timer to uv_check is simple and clearly understandable rather than it.

@tjfontaine
Copy link

I do not disagree there is a deficiency in uv timers, and I'm sure they'll want to address that, but I think simply fixing my 1ms requeue mistake is sufficient compared to making setImmediate behave differently than the other timers.

But I yield to @bnoordhuis and his judgement.

@shigeki
Copy link
Author

shigeki commented Aug 17, 2012

But I yield to @bnoordhuis and his judgement.

Me, too.

@shigeki
Copy link
Author

shigeki commented Aug 17, 2012

I tried to put start_id on timer handles at every uv_timer_start for the second index to compare each elements in RBTree of timers but I found that moving setImmediate from timer to uv_check is simple and clearly understandable rather than it.

The patch is here. shigeki/libuv@50d55ed
But I don't think it is sophisticated.


if (need_immediate_cb) {
uv_check_start(&check_immediate_watcher, node::CheckImmediate);
uv_unref((uv_handle_t*) &check_immediate_watcher);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this immediately after the call to uv_check_init().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@bnoordhuis
Copy link
Member

I'm not sure what the best behavior is here. Homogeneity is important (i.e. don't be different from other timers) but having a check watcher makes it nicely predictable.

@isaacs, @piscisaureus Opinions?

@shigeki
Copy link
Author

shigeki commented Sep 13, 2012

I've just rebased this to the current master and fixed a small bug.
@isaacs @piscisaureus @bnoordhuis Any more opinions? I'm sure that moving setImmediate to uv_check() is beneficial because it avoids future issues caused by racing conditions between timers and setImmediate.

@shigeki
Copy link
Author

shigeki commented Feb 5, 2013

@bnoordhuis node-v0.10 is around the corner. I wonder if this 1msec delay on setImmediate() cause bugs, they would be hard for users to be identified. I will not hold this PR any more and @tjfontaine 's patch for not using a repeat timer is still valid. So please fix it before releasing node-v0.10.

@shigeki shigeki closed this Feb 5, 2013
@bnoordhuis
Copy link
Member

For the record, I like the idea of a check handle (pending joyent/libuv#699) and no one seems to really disagree. I move to go with your approach.

@bnoordhuis bnoordhuis reopened this Feb 5, 2013
@shigeki
Copy link
Author

shigeki commented Feb 6, 2013

Oh, thanks very much, Ben. I'm going to rebase this to work on the current master.

Shigeki Ohtsu added 2 commits February 6, 2013 11:13
uv_check is the robust place to invoke setImmediate callbacks after
process.nextTick and before timers(setTimeout/setInterval)
@isaacs
Copy link

isaacs commented Feb 16, 2013

Landed on master. Thanks, @shigeki! By the way, I really love those process loop diagrams. Do you have them collected somewhere? Can we use them in the node documentation?

@bnoordhuis As far as I can tell, libuv#699 is an issue whether we land this or not, and we can fix it whenever. Moving forward.

@isaacs isaacs closed this Feb 16, 2013
@shigeki
Copy link
Author

shigeki commented Feb 16, 2013

@isaacs Thanks. It's my pleasure to have your offer. I wrote a kind of more simpler diagram in the process.nextTick chapter of a Node book in Japanese for node-v0.8. I will check it again and submit PR to a doc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants